Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Finish adding type hints to the HomeServer #9374

Closed
wants to merge 11 commits into from
Closed

Conversation

clokep
Copy link
Member

@clokep clokep commented Feb 10, 2021

There were a few spots that we didn't have return types on HomeServer this adds them! Unfortunately there's a few spots where we return different types, which makes the code rather messy. I'm not 100% sure this is worth it, although it did actually find a couple of discrepancies.

Comment on lines +241 to +246
if user_device_resync is None:
failures[destination] = {
"status": 503,
"message": "Unable to resync devices",
}
continue
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this method assumes that user_device_resync will raise if the host cannot be reached, but it actually eats errors and returns None instead. 😢

@clokep clokep requested a review from a team February 11, 2021 17:19
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from the one Q

device_handler = hs.get_device_handler()
assert isinstance(device_handler, DeviceHandler)
self._device_handler = device_handler
self._device_handler = hs.get_device_handler()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So something was trying to construct this on a worker? Can we make it so that that doesn't happen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look back into it!

@clokep
Copy link
Member Author

clokep commented Mar 9, 2021

This annoying ends up with circular includes, so...need to figure out what to do about that. 😢

@clokep
Copy link
Member Author

clokep commented Mar 23, 2021

Deleting this since it is easier to do it in bits and pieces.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants